Repository navigation
refactor: QA pass on the skills and agents - #2
Conversation
- pgsql-reviewer: the checklist names each check and points to the skill that owns the rule instead of restating it; the pgx cue flags only a QueryExecMode the pooler cannot serve; an identifier from input needs an allowlist (quoting alone is not enough); squawk rule ids are cited only where squawk ran. - pgsql-plan-analyst: triggers on a plan the assistant just produced; falls back to skills/pgsql-performance/scripts/plan-summary.py under the plugin root when no skill base directory is printed. - Both: EXPLAIN ANALYZE only on a SELECT the caller allowed; never DDL, DML, ANALYZE, or EXPLAIN ANALYZE of a write. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Remove about 40 rules stated in full in two places (a skill and its
reference, or two skills); the second place keeps a pointer.
- Descriptions quote the questions users ask ("why is this query slow",
"which index should I add", "is this migration safe to run in
production"); all stay under 1,024 characters.
- "(checked)" in rule text, the version and date in ## Sources.
- pgsql-migrations: squawk 2.70.0 already flags a missing lock or
statement timeout; what it cannot see is the retry after a timeout.
- pgsql-lint-setup: plpgsql_check reports injection only with
security_warnings => true.
- pgsql-query: the isolation retry is SQLSTATE 40001; a deadlock
(40P01) can happen at any isolation level.
- pgsql-indexing: a range on the plain column needs no expression index
for created_at::date.
- Smaller fixes: a wrong cross-reference, an uncited PostgreSQL 18 claim,
the fillfactor wording, a GIN operator-class wording.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sebastian-iancu
left a comment
sebastian-iancu
left a comment
There was a problem hiding this comment.
Review
No critical defects. The correctness fixes check out: a default GIN index is jsonb_ops; UNIQUE NULLS NOT DISTINCT is scoped to one column; plpgsql_check_function is called with security_warnings => true; 40001 is the isolation retry and 40P01 can happen at any level; a range on the bare created_at column needs no expression index. Skill descriptions are third person, quote real questions, and all eight measure under 1,024 characters (957–1,000). scripts/validate.py passes. The .squawk.toml block in pgsql-lint-setup still matches references/squawk.toml. Agent frontmatter is sound: model: inherit, distinct colors, tools: allowlists with no Write/Edit/Agent, and prose "When to invoke" sections.
Important
1. EXPLAIN ANALYZE of a write is forbidden and prescribed in the same change.
Both agents now say: EXPLAIN (ANALYZE, BUFFERS) only on a SELECT the caller allowed; never DDL, DML, ANALYZE, or EXPLAIN ANALYZE of a write.
These still say to wrap the write and roll it back:
rules/pgsql-context.mdc("Running SQL against a database"), which this PR editedskills/pgsql-performance/SKILL.mdskills/pgsql-performance/references/explain.md, including a workedUPDATEagents/pgsql-plan-analyst.mdedge case "Estimates only": ask forEXPLAIN (ANALYZE, BUFFERS), "run insideBEGIN ... ROLLBACKfor a write"
Step 1 of both agents loads pgsql-performance and reads explain.md. Nothing says the agent's operating rule wins over that recipe, and the plan-analyst's own edge case contradicts its operating rule. The skill's recipe is right for a session the user is driving. One clause on the agent side ("ask the caller to run a write inside BEGIN/ROLLBACK; do not run it yourself") and one on the rule ("the two agents do not run it") would close it. "Never ANALYZE" also sits next to step 6's "ANALYZE then the same EXPLAIN" as the caller's next command; "never run ANALYZE" makes that a recommendation rather than a ban on the word.
2. The two agent descriptions are over the 1,024-character cap the PR claims, and CI does not see it.
Measured with frontmatter_value from scripts/validate.py: pgsql-plan-analyst 1,044, pgsql-reviewer 1,148. Every skill is under the cap. The check lives only in validate_skills(), so validate.py stays green. docs/authoring.md states the Agent Skills limit as 1,024 for the always-on description. The reviewer's extra text is the "do not dispatch it beside that reviewer" sentence (already in the body) and then the "Not for" clause and the "When to invoke" pointer, which is the part a truncation would drop. Trim those two descriptions, or teach validate_md_components the same limit and say so in docs/authoring.md.
Suggestions
skills/pgsql-indexing/references/design.md: "Column order and skip scan" no longer states column order, and "Foreign keys" no longer says to index the referencing columns. Both rules are inSKILL.mdwith no pointer back, while "Read when" and the contents list still send those tasks here. "Uniqueness and NULLs" also dropped "nulls are distinct by default"; that sentence now lives only inpgsql-schema/references/constraints.md.skills/pgsql-jsonb/references/functions.md: "JSON_ARRAYAGGneeds the sameCOALESCEasjsonb_agg(SKILL.md)" points at a rule that names onlyjsonb_agg.skills/pgsql-query/references/go-pgx.mdno longer mentionspgx.Identifier.Sanitize(). The skill still states the allowlist rule;php-pdo.mdstill states the PHP twin. One pointer line would keep the Go reference usable on its own.skills/pgsql-migrations/references/patterns.md, "Add a CHECK or a foreign key": the bullet now starts at "squawk:" with no bold lead. The two-stepNOT VALID/VALIDATErule is in the skill and only in the SQL comments here.pgsql-query's description replaced "retry on serialization failure" with "retry on deadlock". The rule covers both SQLSTATEs, and the description is 1,000 characters, so both phrases fit.pgsql-schema/SKILL.mdstill showsplpgsql_check_function('fn(int)')with nosecurity_warnings.pgsql-lint-setupandpgsql-schema/references/functions.mdnow passsecurity_warnings => true.pgsql-reviewerno longer says to mark a plan claim[NEEDS-CONFIRMATION]when no database was named. The generic edge case still has the marker; the specific trigger does not.
The open checklist items are still open: the description changes were not re-run through the evals, and the new "Reviewing SQL" section of the Cursor rule has not been loaded in Cursor.
Recommendation
Fix the two important items before merge. The suggestions can go in the same commit. I would not block on them alone.
|
Review addressed in 1c89018. Every item was checked against the branch first; all held. Important1. Not done: the extra clause in 2. Agent descriptions over 1,024. Fixed: Not done: a validator check for agents. Suggestions (all applied)
Checks: Still open: the eval re-run and loading the rule in Cursor. |
Review of PR #2: - Both agents: never run ANALYZE, DML or EXPLAIN ANALYZE of a write, even inside the BEGIN ... ROLLBACK a skill or rule prescribes; name the command for the caller. The plan analyst asks the caller to run EXPLAIN ANALYZE; the reviewer marks plan claims [NEEDS-CONFIRMATION] again when no database is named. - Agent descriptions trimmed to 996 and 1,002 characters. - Skills: pointers back for column order, foreign-key indexes and NULLs in unique indexes (indexing design.md); JSON_ARRAYAGG's empty input stated with its source (jsonb functions.md, checked on 18.6); pgx.Identifier.Sanitize() restored (go-pgx.md); the NOT VALID lead restored (migrations patterns.md); plpgsql_check_function with security_warnings (schema); "serialization failure" back in the pgsql-query description. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
A quality pass over the 8 skills, the 2 agents and the Cursor rule, checked against the plugin-dev skill and agent guidance. Four reviewers read the files and proposed edits. Each edit was checked against the files and the cited sources before it went in.
Less duplication, same rules. About 40 rules were written out in full in two places, usually a skill and its own reference, or two skills. Each now lives in one place, with a pointer in the other. Every citation stays next to its rule.
mainDescriptions grow on purpose (+506 chars). They now quote real questions, such as "why is this query slow", "which index should I add" and "is this migration safe to run in production". The evals showed most skills seldom loading from a plain question. All descriptions, the two agents' included, stay under 1,024 characters.
Correctness fixes
pgsql-reviewerflagged any pgx statement cache behind a pooler. It now flags only aQueryExecModethe pooler cannot serve, as the skill says.pgsql-revieweraccepted a quoted identifier taken from input. It now requires an allowlist, as the skill does.EXPLAIN ANALYZEonly on aSELECTthe caller approved, and both nameANALYZEas a write.pgsql-migrationssaid a clean squawk run still needs a lock timeout. squawk 2.70.0 already flags a missing timeout (run in Docker), so the real gap is the retry after a timeout fires./pgsql-lint-setup:plpgsql_checkreports injection only withsecurity_warnings => true, so the example now passes it.pgsql-query: the isolation retry is SQLSTATE40001. A deadlock (40P01) can happen at any isolation level.pgsql-indexingandpgsql-performancegave different fixes forcreated_at::date. Both now say a range on the plain column needs no expression index.fillfactorwording, and one GIN operator-class wording.## Sources, as AGENTS.md asks.Other changes
pgsql-plan-analystnow also triggers on a plan the assistant just produced. When no skill base directory is printed (possible on Cursor), it falls back to the summariser path under the plugin root.pgsql-reviewer.Not changed
ON CONFLICTarbiter rule toDO UPDATEwas turned down: the PostgreSQL 18 manual applies it to everyON CONFLICT.[Unreleased] / Added.Checklist
./scripts/validate.sh,python3 scripts/validate.py --selftestand./scripts/hooks-test.shpassclaude plugin validate .passes## Sourcescarries the checked datepython3 scripts/validate.py --check-linkspasses when citations changed (255 of 255)0.1.0is unreleasedhooks/session-start.sh) when components were added or renamed: none added or renamed🤖 Generated with Claude Code